-
Notifications
You must be signed in to change notification settings - Fork 330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Response.json
static method
#1392
Conversation
This commit adds a `Response.json` static method that can be used to create well formed JSON responses will very little effort.
Note that we'd need to fix Web IDL first to allow regular and static operations with the same name:
|
@domenic Do you think that is desirable to change? It may be confusing to have both a prototype and static method with the same name. Maybe this method should be called |
I think it is desirable to change and do not see anything confusing about having such methods, personally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable, but I think it can be improved by making initialize a response also take a body and Content-Type header value (potentially as a tuple?). That would allow for sharing even more steps.
I also wonder what we should do here if browser interest isn't forthcoming. Perhaps there's a way in which we can still have mutual agreement, but it's not exposed in browsers for now. Perhaps this is worth bringing up in the HTML triage call (if it doesn't otherwise get resolved).
Just fyi... I have an implementation of this in Cloudflare Workers just waiting on this to land or at least signal that it's moving forward. |
I don't think we have any signal that this is moving forward in browsers, unfortunately, due to it just being low-priority sugar for browser teams. As such I don't think it'll land in Fetch, at least until a couple of browsers have engineers looking for starter projects. I'd really encourage you to use this as a case study for https://github.com/whatwg/js-hosts . Figure out what sort of documentation you want to create there, which would enable you to land such things with confidence, and go for it! I'm happy to participate and secure guarantees like "Chromium will never ship a |
@domenic Would this get a 👍 from Chromium if we did the implementation work? |
@ricea would be the decider there but I can predict with very high probability he would support it :). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits and request for a follow-up. Shared algorithm looks rather good! Thanks.
This change isn't possible to roll into WPT's idlharness.js test, see web-platform-tests/wpt#34155. Does anyone want to give it a go to update idlharness.js to avoid this problem? |
…stonly Automatic update from web-platform-tests Fetch: add tests for Response.json Context: whatwg/fetch#1392. -- wpt-commits: 27e770bcfbeae534b5d4cbc0a50d2b8247942312 wpt-pr: 32825
…stonly Automatic update from web-platform-tests Fetch: add tests for Response.json Context: whatwg/fetch#1392. -- wpt-commits: 27e770bcfbeae534b5d4cbc0a50d2b8247942312 wpt-pr: 32825
This commit adds support for the `Response.json` static method as specified in whatwg/fetch#1392. All WPTs from web-platform-tests/wpt#32825 pass. Bug: 1305358 Change-Id: Iaafdd514ed12644b6433c02e7d076ce43f51b9ef Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3639064 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/main@{#1019479}
This commit adds support for the `Response.json` static method as specified in whatwg/fetch#1392. All WPTs from web-platform-tests/wpt#32825 pass. Bug: 1305358 Change-Id: Iaafdd514ed12644b6433c02e7d076ce43f51b9ef Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3639064 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/main@{#1019479} NOKEYCHECK=True GitOrigin-RevId: e680cd1bf0f99e44102f8714dcb5f77fba82c5ed
https://bugs.webkit.org/show_bug.cgi?id=240375 Reviewed by Youenn Fablet. This implements the `Response.json` static method, added to the fetch spec in whatwg/fetch#1392. * LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any.serviceworker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any.sharedworker-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/fetch/api/response/response-static-json.any.worker-expected.txt: * Source/WebCore/Modules/fetch/FetchBody.h: * Source/WebCore/Modules/fetch/FetchResponse.cpp: (WebCore::FetchResponse::create): Added this method overload to implement the spec's "initialize a response" algorithm. This algortihm used to be combined with the regular Response creation algorithm which extracts the body, but Response.json cannot use that directly. (WebCore::FetchResponse::staticJson): * Source/WebCore/Modules/fetch/FetchResponse.h: * Source/WebCore/Modules/fetch/FetchResponse.idl: Canonical link: https://commits.webkit.org/261960@main
This commit adds a
Response.json
static method that can be used tocreate well formed JSON responses will very little effort.
The JSON response is not pretty printed.
Closes #1389
Response.json()
denoland/deno#13902Preview | Diff